Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/telegram bot #455

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Feature/telegram bot #455

wants to merge 6 commits into from

Conversation

sentax
Copy link
Collaborator

@sentax sentax commented May 20, 2024

Implemented a feature for monitoring porpuses that uses telegram bot API and django middleware.

2 new env variables added:
TELEGRAM_API_TOKEN : father bot given token
TELEGRAM_CHANNEL_ID : channel id in 2 formats (number[-1001609474777], string["@ChannelID"])

bot should be added to the channel and allowed to send messages in that channel before.

@sentax sentax requested a review from Inspector-Butters May 20, 2024 10:02
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @sentax - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 4 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment on lines +74 to +75
TELEGRAM_CHANNEL_ID = os.environ.get("TELEGRAM_CHANNEL_ID")
TELEGRAM_API_TOKEN = os.environ.get("TELEGRAM_API_TOKEN")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Environment variables should have default values.

Consider providing default values for TELEGRAM_CHANNEL_ID and TELEGRAM_API_TOKEN to avoid potential NoneType errors if the environment variables are not set.

Suggested change
TELEGRAM_CHANNEL_ID = os.environ.get("TELEGRAM_CHANNEL_ID")
TELEGRAM_API_TOKEN = os.environ.get("TELEGRAM_API_TOKEN")
TELEGRAM_CHANNEL_ID = os.environ.get("TELEGRAM_CHANNEL_ID", "")
TELEGRAM_API_TOKEN = os.environ.get("TELEGRAM_API_TOKEN", "")

}
try:
response = requests.post(url, data=payload)
return {"ok":True}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider logging the exception.

In the except block, consider logging the exception to help with debugging in case of failures.

Suggested change
return {"ok":True}
return {"ok": True}
except Exception as e:
logging.error(f"Exception occurred: {e}")
return {"ok": False}

core/telegram.py Outdated
log_hash = hash(message)
current_time = time.time()

# Check if the log has been sent before or if it has been more than 1 hour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (typo): Comment does not match the code.

The comment mentions '1 hour', but the code uses MIN_INTERVAL which is set to 10 seconds. Update the comment to accurately reflect the code logic.

current_time = time.time()

# Check if the log has been sent before or if it has been more than 1 hour
if current_time - log_cache[log_hash] > MIN_INTERVAL:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Potential race condition with log_cache.

Accessing and modifying log_cache without any locking mechanism could lead to race conditions in a multi-threaded environment. Consider using a thread-safe data structure or adding appropriate locking.

Comment on lines +316 to +394
res = self.middleware.log_message("Test log message")
self.assertEqual(res['ok'], True)
res = self.middleware.log_message("Test log message")
self.assertEqual(res['ok'], False)
# delay
time.sleep(TELEGRAM_MIN_LOG_INTERVAL)
res = self.middleware.log_message("Test log message")
self.assertEqual(res['ok'], True)
res = self.middleware.log_message("Test log message")
self.assertEqual(res['ok'], False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): Extract duplicate code into method (extract-duplicate-method)

@sentax sentax force-pushed the feature/telegram-bot branch from ad3bdb7 to 7f69504 Compare May 20, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant